move BeanCacheObservers into the registry#2556
Conversation
|
Warning Rate limit exceeded@rrayst has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 44 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughRefactors bean-registry and YAML parsing: BeanCacheObserver API updated, BeanContainer now tracks produced class and adds produces(), BeanRegistryImplementation moves to runtime observer registration and a single-arg constructor, GenericYamlParser adds decideClazz() and wires BeanRegistryAware before PostConstruct. Multiple call sites updated to explicit register/start flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as GenericYamlParser
participant Registry as BeanRegistryImplementation
participant Container as BeanContainer
participant Observer as BeanCacheObserver
participant Bean
Note over Parser,Registry: YAML parsing and bean creation flow
Parser->>Parser: decideClazz(kind, grammar, node)
Parser->>Bean: createAndPopulateNode(clazz)
Parser->>Registry: return bean (BeanRegistryAware wired)
Registry->>Container: new BeanContainer(definition, bean)
Container->>Container: set clazz (bean.getClass() / decideClazz)
Registry->>Registry: register(name, container)
alt bean produces BeanCacheObserver
Registry->>Registry: add to observers list
end
Registry->>Observer: handleBeanEvent(BeanDefinitionChanged, newBean, oldBean)
Observer->>Registry: isActivatable(BeanContainer)
Note over Registry,Observer: observer-driven activation/async init notifications
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ok-to-test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
43-43: Remove unused CountDownLatch.The
cdlvariable is created but never used, as the synchronization logic has been removed in this refactoring. Additionally, the import on line 23 andInterruptedExceptionin the constructor's throws clause (line 40) are now unnecessary.🧹 Proposed cleanup
Remove the unused variable:
- CountDownLatch cdl = new CountDownLatch(1); -Remove the unused import:
-import java.util.concurrent.CountDownLatch;Remove the unused exception from the throws clause:
- public YamlParser(String resourceName) throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException, IOException, InterruptedException { + public YamlParser(String resourceName) throws ClassNotFoundException, NoSuchMethodException, InvocationTargetException, InstantiationException, IllegalAccessException, IOException {
🤖 Fix all issues with AI agents
In
@annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:
- Around line 260-266: The method handleAsynchronousInitializationResult(boolean
empty) currently ignores its parameter and recalculates the emptiness via
uidsToActivate.isEmpty(); change the implementation to use the provided
parameter by replacing
observer.handleAsynchronousInitializationResult(uidsToActivate.isEmpty()) with
observer.handleAsynchronousInitializationResult(empty) inside the
synchronized(observers) loop so callers' intent is respected and the parameter
is no longer unused.
In @core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java:
- Line 38: Remove the unused import jakarta.annotation.PostConstruct from
DefaultRouter.java; delete the import statement (the line containing
PostConstruct) and run an import organizer/formatter to ensure no other unused
imports remain and compilation stays clean.
🧹 Nitpick comments (4)
annot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.java (1)
103-113: Consider renaming test method to reflect parsing-time validation.The test now verifies that the exception is thrown during
parse(), not duringresolve(). The method namemissingClassFailsFastOnResolveis now misleading.📝 Suggested rename
@Test - void missingClassFailsFastOnResolve() { + void missingClassFailsFastOnParse() { var ex = assertThrows(RuntimeException.class, () -> { BeanRegistry r = parse("""core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (1)
178-184: Consider extracting duplicate registry initialization logic.The lazy initialization pattern in
getRegistry()duplicates the logic ininit(). While this provides safe access beforeinit()is called, consider extracting to a private helper method to avoid drift.♻️ Suggested refactor
+ private BeanRegistry ensureRegistry() { + if (registry == null) { + registry = new BeanRegistryImplementation(null); + registry.register("router", router); + } + return registry; + } + public void init() { log.debug("Initializing."); - if (registry == null) { - registry = new BeanRegistryImplementation(null); - registry.register("router", router); - } + ensureRegistry(); registry.registerIfAbsent(HttpClientConfiguration.class, HttpClientConfiguration::new); // ... rest of init } public BeanRegistry getRegistry() { - if (registry == null) { - registry = new BeanRegistryImplementation(null); - registry.register("router", router); - } - return registry; + return ensureRegistry(); }annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (1)
30-33: UseClass<?>instead of rawClass.Using the raw
Classtype generates compiler warnings and bypasses generic type safety. Consider parameterizing with a wildcard.♻️ Suggested fix
/** * The class of the bean. */ - private final Class clazz; + private final Class<?> clazz;annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
102-104: Use parameterized logging instead of string concatenation.String concatenation in
log.debugevaluates even when debug logging is disabled, causing unnecessary overhead.♻️ Suggested fix
if (bc.produces(BeanCacheObserver.class)) { observers.add((BeanCacheObserver) bc.getOrCreate(this, grammar)); - log.debug("Registered BeanRegistry observer: " + bc); + log.debug("Registered BeanRegistry observer: {}", bc); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCacheObserver.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.javaannot/src/test/java/com/predic8/membrane/annot/YAMLBeanParsingTest.javaannot/src/test/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementationTest.javaannot/src/test/java/com/predic8/membrane/annot/util/YamlParser.javacore/src/main/java/com/predic8/membrane/core/cli/RouterCLI.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerHealthMonitor.javacore/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.javacore/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.javacore/src/test/java/com/predic8/membrane/core/router/TestRouter.java
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:51.595Z
Learning: In BeanRegistryImplementation, the `getOrCreate(BeanRegistry, Grammar)` method on BeanContainer is guaranteed to always return a non-null object or throw an exception, so null filtering after calling getOrCreate is not necessary.
📚 Learning: 2026-01-03T19:24:51.595Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:51.595Z
Learning: In BeanRegistryImplementation, the `getOrCreate(BeanRegistry, Grammar)` method on BeanContainer is guaranteed to always return a non-null object or throw an exception, so null filtering after calling getOrCreate is not necessary.
Applied to files:
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.javaannot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.javaannot/src/test/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementationTest.javacore/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.javacore/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.javaannot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java
📚 Learning: 2026-01-03T19:24:48.014Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:48.014Z
Learning: In BeanRegistryImplementation.java, the getOrCreate(BeanRegistry, Grammar) method on BeanContainer is guaranteed to return a non-null object or throw an exception. Do not perform or rely on null-filtering after calling getOrCreate; remove any post-call null checks and rely on the contract. If you need extra safety, consider an assertion or explicit exception path for unexpected nulls, but prefer not to branch on null after getOrCreate.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (22)
annot/src/test/java/com/predic8/membrane/annot/util/YamlParser.java (1)
50-54: LGTM! Clean refactoring to simplified initialization pattern.The constructor simplification and explicit
start()call align well with the PR's goal of moving observer registration into the registry itself. The initialization sequence is clear and correct.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/BalancerHealthMonitor.java (3)
53-53: LGTM! Dual initialization pattern for Spring and BeanRegistry.The class now correctly implements both
ApplicationContextAwareandBeanRegistryAware, enabling it to work in both Spring-managed and YAML/BeanRegistry-managed contexts.
168-177: LGTM! Registry-based router injection with lifecycle annotations.The
setRegistrymethod correctly fetches theRouterfrom the registry, and the@PostConstructannotation ensuresinit()is called in non-Spring contexts. TheorElseThrow()is appropriate here since a missing router is a fatal configuration error.
179-190: LGTM! PreDestroy annotation for proper cleanup.Adding
@PreDestroyensures thedestroy()method is invoked when the bean is managed by the YAML-based registry, matching theDisposableBeanbehavior in Spring.annot/src/main/java/com/predic8/membrane/annot/yaml/GenericYamlParser.java (2)
143-156: LGTM! Clean extraction of class resolution logic.The
decideClazzmethod provides a reusable way to determine the target class for a YAML node without instantiation. This supports the newBeanContainer.produces()capability referenced in other files.
316-318: LGTM! BeanRegistry injection before @PostConstruct.The registry is correctly injected into
BeanRegistryAwarebeans before invoking@PostConstructmethods, ensuring beans can access the registry during initialization.core/src/test/java/com/predic8/membrane/core/router/TestRouter.java (2)
44-44: LGTM! Updated to new BeanRegistryImplementation constructor.The single-argument constructor aligns with the refactored
BeanRegistryImplementationthat now accepts only aGrammar(or null for test scenarios).
68-72: LGTM! Explicit router registration pattern.Explicitly registering the router in
init()follows the new pattern where the router is registered after registry creation rather than passed via constructor. This aligns with changes inDefaultMainComponentsandKubernetesWatcher.core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (1)
392-394: LGTM! Improved type-checking with BeanContainer.produces().The signature change from
BeanDefinitiontoBeanContainerwithbc.produces(Proxy.class)is cleaner than the previous grammar-based lookup. This leverages the new type-tracking capability inBeanContainer.core/src/main/java/com/predic8/membrane/core/router/DefaultMainComponents.java (1)
68-74: LGTM! Explicit router registration in init().The registry is now created with a null grammar and the router is explicitly registered afterward. This follows the new pattern where router injection is decoupled from registry construction.
core/src/main/java/com/predic8/membrane/core/kubernetes/KubernetesWatcher.java (1)
50-52: LGTM! Updated to new BeanRegistryImplementation pattern.The registry is now constructed with only the
Grammar, and the router is explicitly registered afterward. This aligns with the broader refactoring to decouple router injection from registry construction.core/src/main/java/com/predic8/membrane/core/cli/RouterCLI.java (1)
166-177: LGTM! Clean initialization sequence.The updated flow correctly separates registry construction from dependency registration. The sequence (create registry → register router → parse YAML → finish static config → start registry → start router) ensures beans can reference the router during resolution.
annot/src/test/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementationTest.java (1)
29-34: Test setup correctly mirrors the new API.The test adapts well to the new constructor signature. Note that the
awaremock is registered but not verified in the current tests—consider removing it if it's no longer needed, or adding assertions if it should be invoked.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanContainer.java (2)
42-54: Constructor changes look correct.Both constructors properly initialize the
clazzfield—either viadecideClazzfor deferred instantiation or viagetClass()for pre-existing singletons. This enables the type-awareproduces()check.
127-132: LGTM onproduces()method.The assignability check is correctly oriented:
clazz.isAssignableFrom(this.clazz)returns true when the bean's type is a subtype of the requested type.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanCacheObserver.java (2)
38-43: Parameter rename improves clarity.Renaming
beantonewBeanbetter distinguishes it fromoldBeanand aligns with the ADD/MODIFY/DELETE semantics documented in the Javadoc.
49-52: Breaking interface change confirmed:isActivatablenow takesBeanContainer.The sole implementation (DefaultRouter) has been properly updated to use the new signature and correctly leverages
BeanContainer.produces(Class)for type-aware filtering of Proxy beans.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (5)
47-47: Synchronized list for observers is appropriate.Using
Collections.synchronizedListallows thread-safe add operations during bean processing. The manual synchronized blocks inisActivatable,handleAsynchronousInitializationResult, andhandleBeanEventare correct for iteration.
73-75: Simplified constructor supports dynamic observer registration.The single-argument constructor aligns with the new design where observers are discovered and registered dynamically when beans producing
BeanCacheObserverare encountered.
99-110: Auto-registration of observers based on bean type.The
produces(BeanCacheObserver.class)check elegantly discovers observers at runtime. However, instantiating the observer viagetOrCreateduringhandleBeanContainerChangemeans the observer is created before the full bean graph may be available.Ensure that observer beans don't depend on other beans that haven't been registered yet. If observers need dependencies, consider deferring their instantiation until
start().
171-176: Double-checking withproduces()andisInstance()is redundant but defensive.
getBeansnow filters byproduces(clazz)on Line 171, then also checksclazz::isInstanceon Line 174. TheisInstancecheck is technically redundant givenproduces(), but serves as defensive coding against edge cases wheregetOrCreatemight return a different type (e.g., proxies). This is acceptable.
189-205: LGTM onregister()method.The method correctly creates a
BeanContainerwith the singleton and delegates tohandleBeanContainerChange, ensuring consistent handling for both programmatically registered beans and YAML-defined beans.
|
@coderabbitai please review |
|
@rrayst I'll review the changes in this PR now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:
- Around line 79-82: The start() method currently iterates all BeanContainer
instances and calls bc.getOrCreate(this, grammar) without excluding component
beans, causing component beans to be instantiated at startup contrary to
getBeans() filtering; update start() to skip containers whose
BeanDefinition.isComponent() is true (i.e. only call getOrCreate for
non-component bean definitions) so behavior matches getBeans(), or if
intentional keep a clear comment and document why component beans need eager
init; locate the loop over BeanContainer in start(), check how
BeanDefinition.isComponent() is accessed elsewhere (e.g., getBeans()), and apply
the same filtering before invoking BeanContainer.getOrCreate(this, grammar).
🧹 Nitpick comments (1)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (1)
99-110: Consider error handling for observer registration.The eager creation of
BeanCacheObserverinstances at line 103 has no error handling. If observer initialization fails (e.g., due to unmet dependencies or construction errors), it will propagate and potentially break the entire bean registration flow.Consider wrapping the observer creation in a try-catch to:
- Log the failure with context
- Decide whether to fail-fast or continue without that observer
- Provide better diagnostics for initialization issues
🛡️ Proposed error handling
private void handleBeanContainerChange(WatchAction action, BeanContainer bc) { bcs.put(bc.getDefinition().getUid(), bc); if (bc.produces(BeanCacheObserver.class)) { - observers.add((BeanCacheObserver) bc.getOrCreate(this, grammar)); - log.debug("Registered BeanRegistry observer: " + bc); + try { + BeanCacheObserver observer = (BeanCacheObserver) bc.getOrCreate(this, grammar); + observers.add(observer); + log.debug("Registered BeanRegistry observer: {}", bc); + } catch (Exception e) { + log.error("Failed to create and register observer for bean {}/{}: {}", + bc.getDefinition().getNamespace(), bc.getDefinition().getName(), e.getMessage(), e); + throw new RuntimeException("Observer registration failed", e); + } } if (!bc.getDefinition().isComponent() && isActivatable(bc)) { uidsToActivate.add(new UidAction(bc.getDefinition().getUid(), action)); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.javacore/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:51.595Z
Learning: In BeanRegistryImplementation, the `getOrCreate(BeanRegistry, Grammar)` method on BeanContainer is guaranteed to always return a non-null object or throw an exception, so null filtering after calling getOrCreate is not necessary.
📚 Learning: 2026-01-03T19:24:48.014Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2529
File: annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java:168-172
Timestamp: 2026-01-03T19:24:48.014Z
Learning: In BeanRegistryImplementation.java, the getOrCreate(BeanRegistry, Grammar) method on BeanContainer is guaranteed to return a non-null object or throw an exception. Do not perform or rely on null-filtering after calling getOrCreate; remove any post-call null checks and rely on the contract. If you need extra safety, consider an assertion or explicit exception path for unexpected nulls, but prefer not to branch on null after getOrCreate.
Applied to files:
annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/router/DefaultRouter.java (1)
391-393: LGTM! Signature aligned with new bean container model.The method signature and implementation correctly align with the new
BeanContainer-based activation model. Usingbc.produces(Proxy.class)provides a cleaner, more direct way to check bean compatibility.annot/src/main/java/com/predic8/membrane/annot/beanregistry/BeanRegistryImplementation.java (4)
73-75: LGTM! Constructor simplified for runtime observer discovery.The constructor signature change removes the need for external observer injection, aligning with the PR objective of moving observers into the registry for dynamic discovery.
169-177: LGTM! Cleaner filtering using BeanContainer API.The change from
bd.getDefinition().produces(clazz)tobd.produces(clazz)at line 171 simplifies the filtering logic and properly delegates to theBeanContainerAPI.
189-205: LGTM! Registration flow aligned with new architecture.The
registermethod now correctly creates aBeanContainerwith aBeanDefinitionand delegates tohandleBeanContainerChange, maintaining consistency with the unified bean registration flow.
245-277: LGTM! Observer notification methods properly synchronized.The new helper methods correctly:
- Synchronize access to the observers list
- Iterate over all registered observers
- Delegate lifecycle events appropriately
- Use the provided parameters (previous issue with unused parameter is resolved)
The addition of
IOExceptiontohandleBeanEventis appropriate for bean lifecycle operations that may involve I/O.
|
This pull request needs "/ok-to-test" from an authorized committer. |
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.